feat(ambassador): add support for provider specific annotations #4120

fad3t · 2023-12-17T15:29:12Z

Description
This change adds support for provider-specific annotations for the ambassador source.

k8s-ci-robot · 2023-12-17T15:29:21Z

Hi @fad3t. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mloiseleur · 2023-12-22T12:57:50Z

@fad3t Would you please :
0. Detail and document the provider specific annotations you would want to use

Add test case with annotations on this source

fad3t · 2023-12-23T15:18:09Z

Hi @mloiseleur,

Sure, at the moment I'm mostly interested by the Cloudflare proxied annotation.
I did my best adding some tests (also trying to stick with standards seen in other sources), please don't hesitate to share comments or ideas to improve it further.

Thx,
Fred

fad3t · 2024-01-10T07:38:11Z

Good morning,
Is this OK to test or do I need to provide some more information?
Thx

fad3t · 2024-01-24T09:49:54Z

Hi @mloiseleur did you have any chance to look at this one? Is it OK to test? Thx

mloiseleur · 2024-01-24T10:09:23Z

/ok-to-test

mloiseleur · 2024-01-24T10:10:10Z

@fad3t Sure. It still needs some documentation on this feature.

fad3t · 2024-01-24T14:05:58Z

@mloiseleur do you have any idea of what could be wrong here?

 --- FAIL: TestAmbassadorHostSource (0.00s)
    --- FAIL: TestAmbassadorHostSource/Service_with_load_balancer_hostname (0.71s)
        ambassador_host_test.go:322: Targets expected "8.8.4.4;8.8.8.8", got "dns.google"
        ambassador_host_test.go:322: RecordType expected "A", got "CNAME"

Those tests run OK locally, and the resolveLoadBalancerHostname flag is set to true when calling extractLoadBalancerTargets, so the hostname should have been replaced by the corresponding IPs.

As for the docs, did you have something in mind? I see there's already some docs about annotations, I've simply flagged the fact Ambassador now supports provider-specific annotations.

Thanks

fad3t · 2024-02-12T09:00:33Z

Hello, can anybody help with the failing test (see comment above)? Thx

mloiseleur · 2024-02-12T10:10:03Z

The test has been fixed.
@fad3t : You just need to rebase.

fad3t · 2024-02-12T10:47:34Z

@mloiseleur I tried rebasing but it would ask me to fix conflicts that have nothing to do with my code.. so instead I merged master into my branch and here it is. Yet it looks like it has plenty of changes unrelated to my feature :)
Any recommendation to make this clean?

mloiseleur · 2024-02-12T11:33:11Z

To remove your last merge commit, something like:

git reset --hard HEAD~1
git push

Should do it.

fad3t · 2024-02-12T12:20:58Z

To remove your last merge commit, something like:
git reset --hard HEAD~1
git push
Should do it.

Yeah, had to look at the commit SHA as I didn't squash during the merge.
Anyway now it looks OK and checks are green :) let me know if that's good for you - and thx for the help!

mloiseleur · 2024-02-12T13:10:25Z

That's far better now.
/lgtm

fad3t · 2024-02-25T13:41:18Z

Hey there 👋 anything else I can do to push this forward? Thanks!

szuecs · 2024-03-12T13:54:02Z

/approve

k8s-ci-robot · 2024-03-12T13:54:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [szuecs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fad3t · 2024-03-12T15:26:27Z

Thanks a lot 👍

k8s-ci-robot requested review from johngmyers and szuecs December 17, 2023 15:29

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 17, 2023

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 17, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 23, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2024

fad3t added 4 commits February 12, 2024 11:40

feat(ambassador): add support for provider specific annotations

6cec5dd

test: add tests

d9606de

fix: update test with LB hostname

1315011

docs: provider-specific annotations for ambassador

f721b41

fad3t force-pushed the feat-amb-provider-specific branch from 4ec19f7 to f721b41 Compare February 12, 2024 12:13

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 12, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 12, 2024

test: fix test with LB hostname

13f6df5

k8s-ci-robot assigned mloiseleur Feb 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024

k8s-ci-robot merged commit 3a2da65 into kubernetes-sigs:master Mar 12, 2024
13 checks passed

fad3t deleted the feat-amb-provider-specific branch March 12, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ambassador): add support for provider specific annotations #4120

feat(ambassador): add support for provider specific annotations #4120

fad3t commented Dec 17, 2023

k8s-ci-robot commented Dec 17, 2023

mloiseleur commented Dec 22, 2023

fad3t commented Dec 23, 2023

fad3t commented Jan 10, 2024

fad3t commented Jan 24, 2024

mloiseleur commented Jan 24, 2024

mloiseleur commented Jan 24, 2024

fad3t commented Jan 24, 2024

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024 •

edited

Loading

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024

fad3t commented Feb 25, 2024

szuecs commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

fad3t commented Mar 12, 2024

feat(ambassador): add support for provider specific annotations #4120

feat(ambassador): add support for provider specific annotations #4120

Conversation

fad3t commented Dec 17, 2023

k8s-ci-robot commented Dec 17, 2023

mloiseleur commented Dec 22, 2023

fad3t commented Dec 23, 2023

fad3t commented Jan 10, 2024

fad3t commented Jan 24, 2024

mloiseleur commented Jan 24, 2024

mloiseleur commented Jan 24, 2024

fad3t commented Jan 24, 2024

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024 • edited Loading

fad3t commented Feb 12, 2024

mloiseleur commented Feb 12, 2024

fad3t commented Feb 25, 2024

szuecs commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

fad3t commented Mar 12, 2024

mloiseleur commented Feb 12, 2024 •

edited

Loading